node: remove dependency on wallet backend packages #23019
Conversation
| type NewBackendEvent struct { | ||
| backend Backend | ||
| wg *sync.WaitGroup | ||
| } |
There was a problem hiding this comment.
This type doesn't need to be exported. Also, it is not common to use WaitGroup for response signaling, it might be better to use a channel.
There was a problem hiding this comment.
Good catch re export. Also replaced the wg with a channel.
|
I rebased the branch and did some more testing. The account manager correctly detects the wallet and adds the accounts for clef and a hdwallet. I did send a tx via clef but getting "transaction type not supported" with a ledger which shouldn't be related to this PR. I didn't have a smart card to test with. |
accounts/manager.go
Outdated
| done := make(chan struct{}, len(backends)) | ||
| for _, backend := range backends { | ||
| am.newBackends <- newBackendEvent{backend, done} | ||
| <-done |
There was a problem hiding this comment.
We should also check if the quit is closed, otherwise this can be blocked forever.
There was a problem hiding this comment.
Quit is not closed ever though, it's a chan chan error. You'd need to add a term chan struct{} that gets closed if you want to check for it. Also, I think the block can only happen in am.newBackends <-, since once we read the event, we will surely respond.
There was a problem hiding this comment.
select {
case am.newBackends <- newBackendEvent{backend, done}:
case <-am.term:
return
}
node/node.go
Outdated
| @@ -42,7 +42,8 @@ type Node struct { | |||
| config *Config | |||
| accman *accounts.Manager | |||
| log log.Logger | |||
| ephemKeystore string // if non-empty, the key directory that will be removed by Stop | |||
| keydir string // key store directory | |||
| isKeyDirEphem bool // If true, key directory will be removed by Stop | |||
accounts/manager.go
Outdated
| kind := reflect.TypeOf(backend) | ||
| am.backends[kind] = append(am.backends[kind], backend) | ||
| am.lock.Unlock() | ||
| event.processed <- struct{}{} |
There was a problem hiding this comment.
I think you can do close(event.processed) if there's no actual error being returned.
accounts/manager.go
Outdated
| func (am *Manager) AddBackends(backends ...Backend) { | ||
| done := make(chan struct{}, len(backends)) | ||
| for _, backend := range backends { | ||
| am.newBackends <- newBackendEvent{backend, done} |
accounts/manager.go
Outdated
| @@ -93,6 +106,16 @@ func (am *Manager) Config() *Config { | |||
| return am.config | |||
| } | |||
|
|
|||
| // AddBackends starts the tracking of additional backends for wallet updates. | |||
| // cmd/geth assumes once this func returns the backends have been already integrated. | |||
| func (am *Manager) AddBackends(backends ...Backend) { | |||
There was a problem hiding this comment.
I think it would be cleaner to have just AddBackend. The constructor needs all the backends in one batch, but if we support adding them from the outside async, might as well add one-by-one.
|
Updated with the suggested reviews |
fjl
changed the title
* accounts: new AddBackends method in manager * node,cmd/geth: mv accman backend init to cmd/geth * node,cmd/geth: mv scrypt config downstreawm from node * accounts: use static buffer size for accman sub chan minor fix * accounts,cmd/geth: update accman backends through its event loop * accounts,node: add comments * accounts: un-export newBackendEvent * accounts: use chan instead of wg in newBlockEvent * node: rename isKeyDirEphem * accounts,cmd: AddBackends->AddBackend * accounts: fix potential blocking when adding backend
* accounts: new AddBackends method in manager * node,cmd/geth: mv accman backend init to cmd/geth * node,cmd/geth: mv scrypt config downstreawm from node * accounts: use static buffer size for accman sub chan minor fix * accounts,cmd/geth: update accman backends through its event loop * accounts,node: add comments * accounts: un-export newBackendEvent * accounts: use chan instead of wg in newBlockEvent * node: rename isKeyDirEphem * accounts,cmd: AddBackends->AddBackend * accounts: fix potential blocking when adding backend
After this PR those importing
github.com/ethereum/go-ethereum/nodewon't need to pull ingithub.com/ethereum/go-ethereum/accounts/*.API changes:
Nodehas an additional methodKeyStoreDirNodeConfig,AccountConfigis replaced byKeyDirConfig(which doesn't return scrypt config)Manager'sBackendsmethod now needs to acquire a read lock